Resolving Potential Evaluation Errors #1553
Conversation
|
This PR resolves a lot of warnings that were being emitted by the IDAES diagnostics toolbox and were being ignored until now. |
| @@ -1459,7 +1461,7 @@ def Dissociation_rule(self, t): | |||
|
|
|||
| # Equation from [2] | |||
| def CO2_acid_base_equilibrium_rule(self, t): | |||
| return pyo.log(10**-self.pK_a_co2) == ( | |||
| return pyo.log(10**-self.pK_a_co2 + eps) == ( | |||
There was a problem hiding this comment.
This constraint looks really odd to me - why not do -pK_a_CO2 == -6.35*exp(7646/R*(1/Tref-T)) (or something to that effect)? At the moment you are taking the log of an exponent which seems to be redundant.
Also, is the eps really needed? If you are trying to avoid evaluation errors then bounding the terms in the logs and divisions would generally be sufficient.
There was a problem hiding this comment.
We could simplify it to log(10) * -pK_a_CO2 == ... in which case the eps probably wouldn't be needed. The LHS wouldn't just be pK_a_co2 because log is actually the natural log, right?
pK_a_co2 currently is bounded in the domain of PositiveReals, but it was still getting a potential evaluation error
There was a problem hiding this comment.
You are correct that I was missing something: the correct constraint would be something like -pK_a_CO2 == -6.35 + log10(exp(7646/R*(1/Tref-T))). However, I think change of base rules tells us that log10(exp(x)) = C*x where C is a base conversion factor.
| return blk.TSS_rem / (pyunits.kg / pyunits.m**3) / 100 / blk.f_thick[t] | ||
| eps = 1e-30 * pyunits.m**3 / pyunits.kg | ||
| return ( | ||
| blk.TSS_rem / (pyunits.kg / pyunits.m**3) / 100 / (blk.f_thick[t] + eps) |
There was a problem hiding this comment.
Would bounding f_thick resolve this issue? As it stands you are adding an effective bound of 1e-30 (which I will note is effectively meaningless unless you have some massive scaling factors applied).
There was a problem hiding this comment.
f_thick is an Expression, so I can't apply a bound to it directly. And adding a constraint to bound it seems like it could be more trouble than it's worth
There was a problem hiding this comment.
Well, that touches on the heart of the issue. To avoid evaluation errors you need to reformulate issues at the root cause. So here your options are to reformulate the constraint that is using this Expression such that f_thick is moved to the other side of the equation if possible, or you really do need some sort of intermediate variable to keep things bounded (or decide it is an acceptable issue which is sometimes unavoidable).
There was a problem hiding this comment.
Alright, in this situation I can just rearrange it such that f_thick is moved to the other side, but in cases where it may not be possible to do this, is it really better to deem it an "acceptable issue" rather than just adding an eps term? Ig what I'm getting at is when should you add an eps term?
There was a problem hiding this comment.
My suggestion is that you should generally not add the eps as it could potentially cause results to change. If your only solution is the add an eps then you have basically already concluded the issue is unavoidable and are just making an arbitrary change to satisfy the diagnostics check. In that case, you would just be better leaving a note that these are known, unavoidable issues.
Fixes/Resolves:
Issue #1547
Summary/Motivation:
Resolves potential evaluation errors across the repo by removing division from constraints and ensuring that we're taking the log of positive numbers
Changes proposed in this PR:
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: